-
Notifications
You must be signed in to change notification settings - Fork 6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[CP][ios][platform_view] workaround for non-tappable webview (#56804) #57032
Closed
hellohuanlin
wants to merge
1
commit into
flutter:flutter-3.28-candidate.0
from
hellohuanlin:cp3.28_pv_webview_not_tappable
Closed
[CP][ios][platform_view] workaround for non-tappable webview (#56804) #57032
hellohuanlin
wants to merge
1
commit into
flutter:flutter-3.28-candidate.0
from
hellohuanlin:cp3.28_pv_webview_not_tappable
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is pretty tricky bug - I suspect that because Apple's internal recognizer caching an outdated state of our delaying recognizer. The conflict happens between WebKit and platform view's recognizers. It happens to all plugins that uses a WKWebView, or any view that has a similar (unknown) gesture setup. This fix has to be in the engine (rather than the plugin), because the plugin itself knows nothing about the existence of our delaying recognizer. Here are the steps of my research for future reference: 1. The bug only happens when the overlay flutter widgets blocks the gestures for the platform view (e.g. tap on the platform view area when a flutter context menu is displayed). When the bug happens, WKWebView's link doesn't work anymore, however, the link can still be highlighted when tapped. 2. When I remove the delaying recognizer from platform view, the link became clickable again. This means that the bug is related to some conflict between WKWebView's internal recognizers and our delaying recognizer. This also means that it is not possible to fix this issue at plugin level, which knows nothing about the delaying recognizer. 3. When we tap on the web view when context menu is displayed, `blockGesture` will be called, which simply toggles delaying recognizer's state from `possible` to `ended` state, meaning it should block all recognizers from the current gesture (and it correctly did so). Then I use `dispatch_async` and check the state again, and confirmed the state is correctly reset to `possible` state. 4. Subsequent tap on web view triggers `acceptGesture`, which turns the `possible` state into `failed` state. This subsequent tap only highlights the link, but not activate the link. This suggests that some internal web kit recognizer that handles the highlight sees the `failed` state of delaying recognizer (which is correct), but the recognizer that handles the link activation probably sees the stale state of `possible` or `ended` (which is outdated old state). 5. So the solution is trying to make the recognizer "see" the updated state rather than the cached old state. 6. I tried recreating a new delaying recognizer when `blockGesture` is called: ``` - blockGesture { delayingRecognizer.state = .ended dispatch_async { // force re-create the delaying recognizer } } ``` This fixed the link activation bug, however, when opening the context menu again, the gesture is not blocked anymore. This means web kit internal recognizers likely cache the old delaying recognizer for state update, thus the new instance of delaying recognizer won't work anymore. So we can't change the instance. However, it's a good experiment that confirms my hypothesis that some internal webkit recognizer caches the outdated state of delaying recognizer. 7. For the above code, rather than using `dispatch_async`, I also tried `dispatch_after`, and it turns out that it only works if the dispatch_after delay is `0` - even if the delay is much smaller than 1 frame's time (16.7ms), it doesn't work. This means the state checking happens either at the end of the current run loop, or beginning of the next run loop. (not too important information, but it helps me better understand how UIKit works). 8. So from 6, we know that we have to keep the original instance of delaying recognizer. I tried toggling `recognizer.enabled`, it didn't work. I also tried inserting a dummy recognizer, it didn't work. Neither approach triggers the state "refresh" for those webkit internal recognizers. 9. I tried removing and adding back the delaying recognizer, and it just worked! This means that removing and adding back the delaying recognizer probably triggered UIKit to refresh the states for all its related recognizers (i.e. those recognizers either blocking or being blocked by the delaying recognizer), hence getting the updated state. *List which issues are fixed by this PR. You must list at least one issue.* Fixes flutter/flutter#158961 *If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].* [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
Is it possiable cherry-picker to 3.24 and provide a new hot fix ? |
@hellohuanlin this did not make it into the initial beta, but will be included in the next hotfix. |
I'm going to update the merge target from main -> flutter-3.28-candidate.0 |
christopherfujino
changed the base branch from
main
to
flutter-3.28-candidate.0
December 12, 2024 21:46
Closing this in favor of another CP |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is CP for #56804 on 3.28. Another CP for 3.27 is here
This is pretty tricky bug - I suspect that because Apple's internal recognizer caching an outdated state of our delaying recognizer.
The conflict happens between WebKit and platform view's recognizers. It happens to all plugins that uses a WKWebView, or any view that has a similar (unknown) gesture setup.
This fix has to be in the engine (rather than the plugin), because the plugin itself knows nothing about the existence of our delaying recognizer.
Here are the steps of my research for future reference:
The bug only happens when the overlay flutter widgets blocks the gestures for the platform view (e.g. tap on the platform view area when a flutter context menu is displayed). When the bug happens, WKWebView's link doesn't work anymore, however, the link can still be highlighted when tapped.
When I remove the delaying recognizer from platform view, the link became clickable again. This means that the bug is related to some conflict between WKWebView's internal recognizers and our delaying recognizer. This also means that it is not possible to fix this issue at plugin level, which knows nothing about the delaying recognizer.
When we tap on the web view when context menu is displayed,
blockGesture
will be called, which simply toggles delaying recognizer's state frompossible
toended
state, meaning it should block all recognizers from the current gesture (and it correctly did so). Then I usedispatch_async
and check the state again, and confirmed the state is correctly reset topossible
state.Subsequent tap on web view triggers
acceptGesture
, which turns thepossible
state intofailed
state. This subsequent tap only highlights the link, but not activate the link. This suggests that some internal web kit recognizer that handles the highlight sees thefailed
state of delaying recognizer (which is correct), but the recognizer that handles the link activation probably sees the stale state ofpossible
orended
(which is outdated old state).So the solution is trying to make the recognizer "see" the updated state rather than the cached old state.
I tried recreating a new delaying recognizer when
blockGesture
is called:This fixed the link activation bug, however, when opening the context menu again, the gesture is not blocked anymore. This means web kit internal recognizers likely cache the old delaying recognizer for state update, thus the new instance of delaying recognizer won't work anymore. So we can't change the instance. However, it's a good experiment that confirms my hypothesis that some internal webkit recognizer caches the outdated state of delaying recognizer.
For the above code, rather than using
dispatch_async
, I also trieddispatch_after
, and it turns out that it only works if the dispatch_after delay is0
- even if the delay is much smaller than 1 frame's time (16.7ms), it doesn't work. This means the state checking happens either at the end of the current run loop, or beginning of the next run loop. (not too important information, but it helps me better understand how UIKit works).So from 6, we know that we have to keep the original instance of delaying recognizer. I tried toggling
recognizer.enabled
, it didn't work. I also tried inserting a dummy recognizer, it didn't work. Neither approach triggers the state "refresh" for those webkit internal recognizers.I tried removing and adding back the delaying recognizer, and it just worked! This means that removing and adding back the delaying recognizer probably triggered UIKit to refresh the states for all its related recognizers (i.e. those recognizers either blocking or being blocked by the delaying recognizer), hence getting the updated state.
List which issues are fixed by this PR. You must list at least one issue.
Fixes flutter/flutter#158961
If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.
Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.
List which issues are fixed by this PR. You must list at least one issue.
If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.
Pre-launch Checklist
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.